Fix(presto)!: preserve SHA256/SHA512 digest semantics, render SHA2 as hex string [CLAUDE]#7824
Conversation
georgesittas
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a few comments.
| if not this.type: | ||
| from sqlglot.optimizer.annotate_types import annotate_types | ||
|
|
||
| this = annotate_types(this, dialect=self.dialect) | ||
|
|
There was a problem hiding this comment.
Let's remove this. For transpilation purposes, nowadays we assume that type inference has ran separately and thus we expect is_type on its own to suffice.
Just need to make sure the Presto functions roundtrip unaffected by default, i.e., when there aren't any type annotations in the AST. I think this is already the case, just double-checking.
| "presto": "LOWER(TO_HEX(SHA512(x)))", | ||
| "trino": "LOWER(TO_HEX(SHA512(x)))", | ||
| }, |
There was a problem hiding this comment.
We should add a test where you annotate types in the ast you get after parsing with bigquery, so that the transpilation path with is_type is reached in a realistic scenario.
| # string-semantics SHA2 renders hex output like md5_sql does for MD5 | ||
| self.validate_all( | ||
| "SELECT SHA2(x, 256)", | ||
| write={ | ||
| "presto": "SELECT LOWER(TO_HEX(SHA256(x)))", | ||
| "trino": "SELECT LOWER(TO_HEX(SHA256(x)))", | ||
| }, | ||
| ) | ||
| self.validate_all( | ||
| "SELECT SHA2(x, 512)", | ||
| write={ | ||
| "presto": "SELECT LOWER(TO_HEX(SHA512(x)))", | ||
| "trino": "SELECT LOWER(TO_HEX(SHA512(x)))", | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
These aren't valid presto inputs afaict, i.e., SHA2(x, 256) and SHA2(x, 512). We should use read={...} and use a dialect that supports these calls to demonstrate actual transpilation so that the test is well-formed.
|
I'll take this to the finish line, thanks. |
|
Thanks for taking it over the line. Both review points noted for next time, and the bigquery SHA512 follow-up offer stands if it's wanted. |
|
No problem! You can see the amended commit where I applied my suggestions here. |
|
On the bigquery SHA512 follow-up: it ripples further than the SHA256 line. Clickhouse needs SHA2Digest wiring, and the new annotated-types test would change expectation, since BigQuery's SHA512 returns BYTES and the value-preserving presto form is SHA512(TO_UTF8(x)) rather than the hex string. That also suggests sha2_digest_sql wants the same typed-text encode handling as sha2_sql. Happy to do all of that as one PR if the direction sounds right. |
|
Go for it and we can chat about the details on the PR :) |
Presto and Trino's native
SHA256/SHA512take and returnVARBINARY, but the parser mapped them toexp.SHA2, the string-to-hex-string expression that MySQL and Spark use. The codebase already models this split withexp.MD5andexp.MD5Digest, and the presto parser maps nativeMD5toMD5Digestone line above the SHA mappings, so this looked like an oversight rather than a decision.Two changes, both mirroring the MD5 precedent:
SHA256/SHA512toexp.SHA2Digest. Digests survive a round trip and transpile to other digest functions: TrinoSHA256(x)becomesUNHEX(SHA256(x))in DuckDB and staysSHA256(x)in BigQuery, instead of silently turning into a hex-string function.exp.SHA2the waymd5_sqlrendersexp.MD5:LOWER(TO_HEX(SHA256(TO_UTF8(x)))), with the UTF8 encode applied when the argument is typed as text. Spark'sSHA2(x, 256)now produces the same value on Trino that it produces on Spark. SHA224/SHA384 raise the usual unsupported warning since Presto has no equivalents.Updated the bigquery and exasol expectations that encoded the old equivalence. Exasol's
HASH_SHA256returns a hex string, soLOWER(TO_HEX(SHA256(x)))is the value-preserving translation there; its presto/trino read entries asserted that a digest function and a hex-string function were the same thing, which is the bug this fixes, so those two entries are removed.Found while chasing a surrogate key mismatch in SQLMesh on Trino (SQLMesh/sqlmesh#5871): MD5-based keys transpile correctly, SHA256-based keys silently change value.
One adjacent inconsistency deliberately left alone: the bigquery parser maps
SHA256toSHA2DigestbutSHA512toSHA2, while BigQuery returnsBYTESfor both. Happy to follow up separately if that is wanted.